-
Notifications
You must be signed in to change notification settings - Fork 132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a new diagnostic variable dvsdtd. #940
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Is there not also a dvsdtt?
These changes will need to be made to all of the drivers, eventually.
@@ -419,6 +420,7 @@ subroutine alloc_flux | |||
strintyU (nx_block,ny_block,max_blocks), & ! divergence of internal ice stress, y (N/m^2) | |||
daidtd (nx_block,ny_block,max_blocks), & ! ice area tendency due to transport (1/s) | |||
dvidtd (nx_block,ny_block,max_blocks), & ! ice volume tendency due to transport (m/s) | |||
dvsdtd (nx_block,ny_block,max_blocks), & ! snow volume tendency due to transport (m/s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I first looked at the PR, I thought this was salt, not snow. Do we need to distinguish the names of those now, in case we want to add a salt tendency term?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it would not be hard to add a dvsdtt term. I was doing it specifically since there is the need for an SIMIP variable sndmassdyn. There is not a similar term for the thermodynamics. The snow melt and snow ice formation are called out in separate terms. I was using dvsdtd to be consistent with dvidtd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In update state, I suggest
- Add "if present" to dagedt calculation
- With "if present", I think this syntax still sometimes creates run time problems with debug flags (it shouldn't)
if (present(xxx)) xxx = yyy
would recommend
if (present(xxx)) then
xxx = yyy
endif
- Can we update the indentation in update_state? Seems it may be overdue.
Since update_state is not backwards compatible, could you update all the call to update_state in all the other drivers. I would NOT add dvsdtd to those calls, but just update the arguments to use keywords= syntax. That should be adequate. Thanks. |
Ah. Good point. I will do that. I will also work on dvsdtt at the same time. |
for snow). Also, the calls to update_state have been updated in all of the drivers. Note that I can only test the NUOPC/CMEPS and standalone drivers, so it would be good if others could test.
I have added dvsdtt and fixed all of the calls to update_state in the drivers. The updated test results for the standalone driver are here: |
Update the opticep unit test to be consistent with the latest changes.
I ran a test suite on Derecho and this all looks good. @dabail10, anything left to do? Any other comments? If not, I'll merge in the next day or two. |
I believe this is everything here. |
* Add dvsdtd diagnostic for SIMIP sndmassdyn * Fix the units of SIMIP snow variables * Fix calls to update_state * Add present for dagedt and fix indents * This PR has been updated to include dvsdtt (thermodynamic tendency for snow). Also, the calls to update_state have been updated in all of the drivers. Note that I can only test the NUOPC/CMEPS and standalone drivers, so it would be good if others could test. * Update the opticep unit test to be consistent with the latest changes. --------- Co-authored-by: apcraig <[email protected]>
For detailed information about submitting Pull Requests (PRs) to the CICE-Consortium,
please refer to: https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers
PR checklist
This adds a new diagnostic variable dvsdtd so that the SIMIP variable sndmassdyn can be used.
dabail10 (D. Bailey)
https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#1db0d9c13c0885c2da5c498a3fee0daa60788434
We finally added the SIMIP variable sndmassdyn. This required the computation of dvsdtd. Also, there were some unit issues with some of the sndmass variables.